Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: improve documentation for writeText* functions #277171

Closed
wants to merge 1 commit into from

Conversation

K900
Copy link
Contributor

@K900 K900 commented Dec 27, 2023

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@mcdonc
Copy link
Contributor

mcdonc commented Dec 27, 2023

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Dec 27, 2023
@K900 K900 requested review from a team December 29, 2023 14:27
Copy link
Contributor

@asymmetric asymmetric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement, left some suggestions, but LGTM.

Note that the output of `writeText` is a derivation, which will coerce to the store path of its output,
which will not match the path of the actual file if `destination` is set.

Many more functions wrap `writeTextFile`, including `writeText`, `writeTextDir`, `writeScript`, and `writeScriptBin`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is already like this in the manual, but instead of including, could we list them all?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no! This documentation has been duplicated across

  • doc/build-helpers/trivial-build-helpers.chapter.md, and
  • pkgs/build-support/trivial-builders/default.nix

We should either use nixdoc (like lib/), or replace all the doc comments by # See doc/build-helpers/trivial-build-helpers.chapter.md.

There's no point doing anything without addressing this duplication first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no point doing anything without addressing this duplication first.

Maybe I should tone that down, but it is a very very serious problem nonetheless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As much as I don't want to throw this over the fence, I'm not sure I have it in me right now to handle a large scale cleanup like this. If anyone's interested, feel free to take over.

destination = "some/subpath/my-cool-script";

# (Optional) Commands to run after generating the file, e.g. lints.
# Defaults to "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Defaults to "";
# Defaults to "".

# (Optional) Commands to run after generating the file, e.g. lints.
# Defaults to "";
checkPhase = ''
${pkgs.shellcheck}/bin/shellcheck $out/some/subpath/my-cool-script
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use destination here to avoid duplication?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to test it if you do.


# (Optional) Whether to prefer building locally, even if faster remote builders are available.
# Defaults to true for similar reasons.
preferLocalBuild = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So does this take precedence on the above, so that if e.g. allowSubstitutes = true && preferLocalBuild = true, the build is kept local?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should refer to the Nix manual and not copy the explanation.
It may be subject to change and documenting it here would suggest that it's unique.

```

Note that the output of `writeText` is a derivation, which will coerce to the store path of its output,
which will not match the path of the actual file if `destination` is set.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example could be nice here IMO.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asymmetric asymmetric requested a review from a team December 29, 2023 16:09
@K900 K900 closed this Dec 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants